Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [#615] i18n support added #627

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Conversation

vivshankar
Copy link
Contributor

The changes are proposed in #615 .

The changes involved:

  • MessageCatalog interface exposes two funcs that can be used to choose an implementation of choice. The DefaultMessageCatalog provides a simple implementation.
  • By default, MessageCatalog is nil and the implementation falls back to current behavior (tested by running the unit tests)
  • WithHint and WithHintf record the original formatted message and arguments for legacy translation. Here the catalog contains a message ID that matches the formatted message.
  • WithHintIDOrDefaultf allows a message ID to be specified along with a default message. If a localizer is provided, the message ID is used to find the message and the vaargs are applied (if present). Otherwise the default formatted message is used.
  • Requesters have been updated to implement a G11NContext interface that declares a GetLang func.

There are no Breaking Changes.

NOTE: Some gaps exist in this implementation with the introspection handler because of a missing requester object. This will be addressed in a future PR.

Related issue(s)

Linked to PR.

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added necessary documentation within the code base (if
    appropriate).

Further comments

The actual translation of messages is left to the point when GetDescription is called. Thus, the localizer only needs to be known to the error object at this point. This allows us to limit the changes to when error responses are built. For example, when WriteAuthorizeError is invoked.

In both the authorize and token flows, the requester object is passed into the Write*Error func making it simple to extend the requester object to implement the G11NContext interface that declares a GetLang function. With this, all that a consumer needs to do is add a MessageCatalog instance to the compose.Config and the changes here take care of the rest.

However, WriteIntrospectionError does not accept the requester and, so, a helper function - AddLocalizerToErrWithLang - is provided to augment the error object with the localizer details (catalog and lang).

For example:

        tt, ar, err := oauth2Provider.IntrospectToken(ctx, token, fosite.TokenType(tokenType),
		session, strings.Split(scope, " ")...)
	if err != nil {
		newerr := fosite.AddLocalizerToErrWithLang(h.s.MessageCatalog(), i18n.GetLangFromRequest(h.s.MessageCatalog(), r), fosite.ErrRequestUnauthorized.WithHintID(i18n.ErrHintInvalidHTTPAuthzHeader))

		oauth2Provider.WriteIntrospectionError(rw, newerr)
		return
	}

This is somewhat cumbersome and should probably be enhanced to pass in the requester to the WriteIntrospectError. In the interest of ensuring backward compatibility, I have chosen not to make these changes but an overload func WriteIntrospectErrorWithRequester can be considered in a future update.

@vivshankar
Copy link
Contributor Author

@aeneasr The errors with the conformity tests appear to be unrelated to this PR.

As per our discussion on Slack, please see this revised PR that provides a method to support ID-less hints for backward compatibility and to control changes.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks much better already than the previous PR :) Maybe you could add a bit of docs to the README to show how one would define translation messages? This would help me also better understand the code!

access_request_handler.go Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Oct 23, 2021

Looks like conformity tests are failing with a build error, could you take a look please?

https://github.com/ory/fosite/runs/3913037380?check_suite_focus=true

@vivshankar
Copy link
Contributor Author

vivshankar commented Oct 24, 2021

Thanks for your review @aeneasr.

The failure appears to be

 ---> Running in 3dfdbcd9fc30
# github.com/ory/hydra/internal/httpclient/models
internal/httpclient/models/accept_consent_request.go:78:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:95:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:112:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:130:14: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:171:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:185:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:199:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_consent_request.go:214:14: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_login_request.go:92:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_login_request.go:129:13: ce.ValidateName undefined (type *"github.com/go-openapi/errors".CompositeError has no field or method ValidateName)
internal/httpclient/models/accept_login_request.go:129:13: too many errors
The command '/bin/sh -c go build -tags sqlite -o /usr/bin/hydra' returned a non-zero code: 2
Service 'hydra-migrate' failed to build : Build failed

I don't think it is related to my changes as such.

Edit: It might have been fixed on the master branch on Hydra. I took a look and the file has changed.

Edit Edit: Bingo! The changes you made did it. All green now

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there :)

errors.go Show resolved Hide resolved
i18n/i18n_test.go Show resolved Hide resolved
i18n_helper.go Outdated
Comment on lines 26 to 28
if e, ok := err.(*RFC6749Error); ok {
return e.WithLocalizer(catalog, lang)
} else if e, ok := errorsx.Cause(err).(*RFC6749Error); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use errors.As instead!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@vivshankar
Copy link
Contributor Author

@aeneasr I have made the changes and also added a i18n_helper_test that tests the usage using an error object.

@aeneasr aeneasr merged commit cf02af9 into ory:master Oct 29, 2021
@aeneasr
Copy link
Member

aeneasr commented Oct 29, 2021

Awesome, great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants